Skip to content

Conversation

@Noroth
Copy link
Contributor

@Noroth Noroth commented Jan 5, 2026

Summary by CodeRabbit

  • New Features

    • Per-field "required fields" for federated entities: generates dedicated RPCs, request/response messages, and mappings.
    • Selection-set normalization for abstract/interface types so interface-level selections are distributed into fragments.
    • Public API to produce full, deterministically ordered proto text from GraphQL.
  • Improvements

    • Modularized proto-generation utilities and expanded naming helpers and directive/constant names.
  • Validation

    • New rule disallowing abstract types inside @requires selections.
  • Tests

    • Large expansion of tests covering field sets, federation, proto output, naming, and validation.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Walkthrough

Adds support for GraphQL federation @requires: new proto message and TS binding RequiredFieldMapping, visitors and validators to normalize/validate required-field selection sets, proto rendering/naming utilities (including list wrappers and composites), integration into SDL→mapping and SDL→proto flows, expanded tests, and CI/lint updates.

Changes

Cohort / File(s) Summary
Proto & generated bindings
proto/wg/cosmo/node/v1/node.proto, connect/src/wg/cosmo/node/v1/node_pb.ts
Adds RequiredFieldMapping message and repeated RequiredFieldMapping required_field_mappings = 7 to EntityMapping; updates generated TS bindings with RequiredFieldMapping class and serialization helpers.
Required-fields pipeline
protographic/src/required-fields-visitor.ts, protographic/src/abstract-selection-rewriter.ts, protographic/src/selection-set-validation-visitor.ts
New visitors: RequiredFieldsVisitor generates per-key RPCs/messages/mappings from @requires; AbstractSelectionRewriter normalizes selections across abstract types; SelectionSetValidationVisitor validates @requires selection sets.
SDL→mapping & validation
protographic/src/sdl-to-mapping-visitor.ts, protographic/src/sdl-validation-visitor.ts
Integrates required-field mapping generation into entity processing, extracts/validates @requires field sets, appends requiredFieldMappings to EntityMapping, and enforces a rule disallowing abstract types in @requires.
SDL→proto refactor & utils
protographic/src/sdl-to-proto-visitor.ts, protographic/src/proto-utils.ts, protographic/src/types.ts
Major refactor centralizing proto utilities (message/RPC rendering, GraphQL→proto type mapping, list-wrapper generation), adds deterministic visit()/visit output, message/RPC collection, wrappers, and integrates required-field RPC/message generation.
Naming, constants & exports
protographic/src/naming-conventions.ts, protographic/src/string-constants.ts, protographic/src/index.ts
Adds key-normalization and naming helpers, new directive/name constants (key, external, requires, fields), and exports RequiredFieldsVisitor plus string-constant symbols.
Proto rendering tests
protographic/tests/proto-utils/build-proto-message.test.ts
Comprehensive tests for buildProtoMessage covering nested wrappers, composites, reserved numbers, comments, deduplication and error cases.
Field-set & federation tests
protographic/tests/field-set/01-basics.test.ts, protographic/tests/sdl-to-mapping/03-federation.test.ts, protographic/tests/sdl-to-proto/04-federation.test.ts
Adds many federation/required-field tests and snapshots verifying RPC/message/mapping generation for @requires (scalar, nested, interface/union, key variations).
Test imports & assertions
protographic/tests/** (many files)
Mass update of test imports to explicit .js module specifiers, dot-notation refactors in assertions, and multiple new/updated tests across suites.
Operations & internals
protographic/src/operations/*, protographic/src/proto-lock.ts, protographic/src/operation-to-proto.ts
Numerous small refactors: iteration style changes, default-parameter/type-annotation simplifications, field-numbering and lock reconciliation tweaks, and import reorderings—mostly internal/stylistic.
Packaging, lint & CI
protographic/package.json, protographic/.eslintrc, protographic/.eslintignore, .github/workflows/protographic.yaml
Adds workspace dep @wundergraph/composition, permissive ESLint config and ignores, adjusts lint scripts, and updates CI to install/build additional workspace packages.
Miscellaneous
multiple protographic/src/* files
Minor formatting, helper additions (VisitContext, RPCMethod, ProtoMessage types), signature default simplifications, and supporting API expansions enabling the new flow.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: add support for requires in protographic' directly and accurately summarizes the main objective of the changeset, which is to implement @requires directive support in the protographic code generation system.
Docstring Coverage ✅ Passed Docstring coverage is 90.70% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 89.75347% with 133 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.59%. Comparing base (f585037) to head (a7264e3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
protographic/src/required-fields-visitor.ts 91.76% 27 Missing ⚠️
protographic/src/operations/message-builder.ts 18.75% 26 Missing ⚠️
protographic/src/sdl-to-proto-visitor.ts 92.98% 19 Missing ⚠️
...otographic/src/selection-set-validation-visitor.ts 84.11% 17 Missing ⚠️
protographic/src/sdl-validation-visitor.ts 90.42% 9 Missing ⚠️
protographic/src/abstract-selection-rewriter.ts 88.88% 8 Missing ⚠️
protographic/src/sdl-to-mapping-visitor.ts 91.66% 6 Missing ⚠️
protographic/src/proto-lock.ts 70.58% 5 Missing ⚠️
protographic/src/operations/field-numbering.ts 55.55% 4 Missing ⚠️
...rotographic/src/operations/proto-text-generator.ts 81.81% 4 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2439       +/-   ##
===========================================
- Coverage   61.63%   40.59%   -21.05%     
===========================================
  Files         229     1019      +790     
  Lines       23894   142726   +118832     
  Branches        0     9127     +9127     
===========================================
+ Hits        14728    57941    +43213     
- Misses       7924    83232    +75308     
- Partials     1242     1553      +311     
Files with missing lines Coverage Δ
protographic/src/index.ts 86.84% <100.00%> (ø)
protographic/src/operation-to-proto.ts 83.79% <100.00%> (ø)
protographic/src/operations/proto-field-options.ts 100.00% <100.00%> (ø)
protographic/src/operations/type-mapper.ts 89.69% <100.00%> (ø)
protographic/src/string-constants.ts 100.00% <100.00%> (ø)
protographic/src/types.ts 100.00% <100.00%> (ø)
router/gen/proto/wg/cosmo/node/v1/node.pb.go 13.75% <ø> (-7.02%) ⬇️
protographic/src/naming-conventions.ts 96.96% <93.10%> (ø)
protographic/src/proto-utils.ts 98.98% <98.98%> (ø)
protographic/src/operations/field-numbering.ts 83.78% <55.55%> (ø)
... and 10 more

... and 874 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link

github-actions bot commented Jan 19, 2026

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-20907784f3d7cbaa268147c0ec31cde00c04275e-nonroot

@Noroth Noroth marked this pull request as ready for review February 3, 2026 10:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Fix all issues with AI agents
In `@protographic/src/abstract-selection-rewriter.ts`:
- Line 57: The currentType property is only set in the constructor and never
updated during AST traversal, so findNamedTypeForField always resolves against
the root type; update currentType as the visitor descends and ascends into
fields: when entering a field (or selection set) look up the field's type from
the previous currentType using the same logic as findNamedTypeForField and set
currentType to the named/unwrapped type, and when leaving the field restore the
previous currentType (e.g., push/pop or store parent type); modify the visitor
methods in the AbstractSelectionRewriter class that handle entering/leaving
fields/selections to maintain currentType correctly so nested selections resolve
to their actual object types.

In `@protographic/src/naming-conventions.ts`:
- Around line 88-110: The doc examples for createRequiredFieldsMethodName are
out of sync with createMethodSuffixFromEntityKey which sorts keys; update the
examples or remove the sort—prefer updating the examples: change the example
showing "name,id" to reflect the sorted suffix (i.e.,
createMethodSuffixFromEntityKey('name,id') => 'ByIdAndName' and
createRequiredFieldsMethodName('User','post','name,id') =>
'RequireUserPostByIdAndName'); reference createRequiredFieldsMethodName and
createMethodSuffixFromEntityKey when making the change.

In `@protographic/src/required-fields-visitor.ts`:
- Around line 496-519: The code in handleCompositeType overwrites
this.current.compositeType for each interface/union field so only the last
composite is kept; change the message model to hold an array (e.g.,
this.current.compositeTypes) and update handleCompositeType to push a new
composite metadata object instead of assigning to compositeType (use
CompositeMessageKind.INTERFACE/UNION, implementingTypes/memberTypes and typeName
as in the diff); also update any consumers/readers of current.compositeType to
iterate the new compositeTypes array (and adjust types/interfaces for the
message object accordingly).
- Around line 439-477: The nested message naming uses the object type name which
can collide when the same object type is used by multiple fields; update
onEnterSelectionSet to derive a unique nested message identifier (e.g., include
the parent field name or path) instead of solely using this.currentType.name,
then check this.current.nestedMessages for an existing nested by that identifier
and reuse it (push existing into stack and set this.current) instead of always
creating a new one; reference functions/vars to change: onEnterSelectionSet,
findObjectTypeForField / findObjectType, this.current, this.currentType,
this.current.nestedMessages, nested (ProtoMessage), this.stack, and
this.current.name to ensure no duplicate nested message definitions.
- Around line 141-152: The visitor currently iterates raw key directive strings
and emits duplicate RPC/messages when keys differ only by order/spacing; update
visit() to normalize and deduplicate key field sets (use the same normalization
that createRequiredFieldsMethodName uses or normalize by parsing, trimming and
sorting field names) by computing a normalizedKey (e.g., from
getKeyFieldsString(keyDirective) then normalize) and maintain a Set of
seenNormalizedKeys; if normalizedKey is already seen skip emitting (skip setting
this.mapping[this.currentKeyFieldsString] and calling visit(this.fieldSetDoc,
this.visitor)), otherwise add to seen and proceed, ensuring you still reference
this.currentKeyFieldsString/getKeyFieldsString and the mapping/visit calls when
emitting the first occurrence.

In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 397-424: validateDisallowAbstractTypesForRequires currently calls
parse(fieldSelections.value) which can throw and abort validation; wrap the
parse call in a try/catch inside validateDisallowAbstractTypesForRequires, and
on parse failure convert the exception into a validation error via this.addError
(include parse error message and ctx.node.loc) instead of rethrowing; only
proceed to construct SelectionSetValidationVisitor, call visit(), and relay its
errors/warnings if parse succeeds.

In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 110-181: getFieldDefinition currently throws when a field isn't
found which aborts validation; change it to return null (or undefined) instead
and push a validation error into this.validationResult.errors (include field
name and this.currentType.name for context). Update the getFieldDefinition
signature to allow null and remove the throw; then update callers — onEnterField
and onEnterSelectionSet — to check the returned value (e.g., if (!fieldDef) {
return; } or push errors as needed) before using fieldDef.type or accessing
properties. Ensure error messages match existing validation style and keep
traversal/validation continuing instead of throwing.

In `@protographic/tests/field-set/01-basics.test.ts`:
- Around line 69-122: The test's `@requires` field set never contains duplicates
so the deduplication logic isn't exercised; update the SDL in this test (the sdl
string used to build the schema in the 'should visit a field set for a scalar
type and deduplicate fields' test) to include a duplicate selection (e.g. change
`@requires`(fields: "name") to `@requires`(fields: "name name") or otherwise repeat
"name") so RequiredFieldsVisitor receives a duplicated field in fieldSet and the
dedupe path is tested.
🧹 Nitpick comments (5)
protographic/src/abstract-selection-rewriter.ts (1)

173-175: Unused private method.

The fieldDefinition method is defined but never called anywhere in this class.

♻️ Remove unused method
-  /**
-   * Retrieves the field definition for a given field name from the current type.
-   *
-   * `@param` fieldName - The name of the field to look up
-   * `@returns` The GraphQL field definition, or undefined if not found
-   */
-  private fieldDefinition(fieldName: string): GraphQLField<any, any, any> | undefined {
-    return this.currentType.getFields()[fieldName];
-  }
protographic/tests/sdl-to-proto/01-basic-types.test.ts (1)

1-1: Unused import: it is imported but not used in this file.

All tests in this file use test(...) syntax. The it import appears unused.

♻️ Remove unused import
-import { describe, expect, it, test } from 'vitest';
+import { describe, expect, test } from 'vitest';
protographic/tests/sdl-to-mapping/03-federation.test.ts (2)

1008-1008: Stale comment - appears to be a leftover from development.

The comment // RequireWarehouseStockHealthScoreByIdFields.RestockData doesn't match the test context (this test uses Product, not Warehouse).

🧹 Remove stale comment
     const mapping = compileGraphQLToMapping(sdl, 'ProductService');

-    // RequireWarehouseStockHealthScoreByIdFields.RestockData
     expect(mapping.toJson()).toMatchInlineSnapshot(`

1103-1103: Stale comment - same leftover as above.

🧹 Remove stale comment
     const mapping = compileGraphQLToMapping(sdl, 'ProductService');

-    // RequireWarehouseStockHealthScoreByIdFields.RestockData
     expect(mapping.toJson()).toMatchInlineSnapshot(`
protographic/src/sdl-to-mapping-visitor.ts (1)

131-141: Remove stale TODO to avoid confusion.

Required-field mapping is now implemented below, so this TODO is misleading.

💡 Suggested cleanup
-            // todo: add required fields mapping

Would you like me to open a small cleanup issue or prep a patch?

private readonly visitor: ASTVisitor;
private readonly fieldSetDoc: DocumentNode;
public readonly schema: GraphQLSchema;
private currentType: GraphQLObjectType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

currentType is never updated during traversal.

The currentType field is set once in the constructor but never updated as the visitor traverses nested fields. This means findNamedTypeForField will always look up fields from the root object type, which may cause incorrect type resolution for deeply nested selections.

🤖 Prompt for AI Agents
In `@protographic/src/abstract-selection-rewriter.ts` at line 57, The currentType
property is only set in the constructor and never updated during AST traversal,
so findNamedTypeForField always resolves against the root type; update
currentType as the visitor descends and ascends into fields: when entering a
field (or selection set) look up the field's type from the previous currentType
using the same logic as findNamedTypeForField and set currentType to the
named/unwrapped type, and when leaving the field restore the previous
currentType (e.g., push/pop or store parent type); modify the visitor methods in
the AbstractSelectionRewriter class that handle entering/leaving
fields/selections to maintain currentType correctly so nested selections resolve
to their actual object types.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
protographic/src/sdl-to-mapping-visitor.ts (1)

130-141: ⚠️ Potential issue | 🟡 Minor

Remove the stale TODO comment.
The required-fields mapping is now implemented below, so this TODO is misleading.

🧹 Suggested cleanup
-            // todo: add required fields mapping

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@protographic/src/abstract-selection-rewriter.ts`:
- Around line 122-142: The code replaces ctx.node.selections with
inlineFragments even when inlineFragments is empty, which drops interface-level
fields; add an early return (or skip the replacement) right after computing
inlineFragments (check inlineFragments.length === 0) to avoid mutating
ctx.node.selections when there are no inline fragments, then only proceed with
distributing fields into fragments (using fields, inlineFragments, this.hasField
and fragment.selectionSet.selections) when fragments exist.

In `@protographic/src/required-fields-visitor.ts`:
- Around line 445-494: The enter/leave imbalance happens because
onEnterSelectionSet sometimes returns before pushing, but onLeaveSelectionSet
always pops; fix by tracking whether a push happened and only pop when it did:
in onEnterSelectionSet compute a local pushed = false and set pushed = true
right before you push this.currentType onto this.ancestors and this.current onto
this.stack (and when you add nested message), then push that pushed flag onto a
new boolean stack (e.g., this.selectionSetPushStack) so the state is paired; in
onLeaveSelectionSet pop the flag from this.selectionSetPushStack and only pop
this.ancestors/this.stack and restore this.currentType/this.current when the
flag is true, leaving them unchanged when false. Ensure the new stack is
initialized on the class and used consistently with the existing ancestors/stack
fields.

In `@protographic/src/sdl-to-mapping-visitor.ts`:
- Around line 187-196: getRequiredFieldSet assumes the directive argument is a
StringValueNode and casts without checking, which yields undefined node.value
for non-string kinds; update getRequiredFieldSet to first inspect the directive
argument's kind (compare to Kind.STRING) before casting to StringValueNode,
throw a clear error when the argument is missing or not Kind.STRING, and ensure
Kind is imported from 'graphql' so the code safely accesses node.value for the
REQUIRES_DIRECTIVE_NAME lookup.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 110-115: When getFieldDefinition returns null in onEnterField,
return false (not undefined) so traversal into that field's subtree is skipped;
similarly, when onEnterSelectionSet cannot resolve the field/type, return false
instead of undefined. Update onEnterField (uses getFieldDefinition) and
onEnterSelectionSet to return false on failed lookups, and modify
onLeaveSelectionSet to only pop from the ancestors/currentType stacks when an
entry was actually pushed (e.g., check ancestors length or a pushed-flag) to
avoid corrupting currentType.
- Around line 110-123: The validator in SelectionSetValidationVisitor (methods
onEnterField and onEnterSelectionSet) wrongly rejects abstract types and inline
fragments for `@requires` field sets; remove or bypass the checks that call
isAbstractType(namedType) and that reject InlineFragment nodes so that abstract
return types (interfaces/unions) and inline fragments are allowed to pass
through to RequiredFieldsVisitor/AbstractSelectionRewriter for normalization.
Specifically, update onEnterField (remove the block that pushes the "Abstract
types are not allowed..." error and returns BREAK for abstract namedType) and
update onEnterSelectionSet (remove or guard the rejection of InlineFragment
nodes) — or alternatively wrap those checks so they are skipped when processing
`@requires` field sets handled by AbstractSelectionRewriter/RequiredFieldsVisitor.

In `@protographic/tests/field-set/01-basics.test.ts`:
- Around line 23-24: The docstring for the entityName field incorrectly claims a
default to the `@key` entity; update the comment for the entityName property (the
entityName field) to remove the "defaults to finding the type with `@key`
directive" phrase and clearly state that entityName is required (or,
alternatively, implement an actual fallback to derive entityName from the `@key`
directive in the code that constructs/validates entityName such as the
factory/parser that populates entityName); prefer the doc-only change: edit the
comment above the entityName declaration to indicate it is required and does not
default to `@key`.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@protographic/src/proto-utils.ts`:
- Around line 289-315: The JSDoc for createNestedListWrapper is incorrect: it
says the function returns the wrapper message name but the implementation
(createNestedListWrapper -> builds wrapperName and returns
buildWrapperMessage(...).join('\n')) actually returns the rendered proto message
string; update the `@returns` description to state it returns the generated
wrapper message definition as a string (e.g., the full proto message text), and
optionally mention that it uses buildWrapperMessage(includeComments,
wrapperName, level, baseType) to render it; keep references to
createNestedListWrapper, wrapperName, and buildWrapperMessage so future readers
can locate the logic.
- Around line 104-141: The oneof field numbers in buildCompositeTypeMessage are
assigned based on the original compositeTypes array order (from memberTypes or
implementingTypes) which is non-deterministic; before iterating, sort
compositeTypes deterministically (e.g., lexicographically by the type string) so
the loop that calls graphqlFieldToProtoField(compositeType) and assigns =
${index + 1} produces stable, repeatable field numbers and avoids
wire-compatibility breaks.
- Around line 149-171: The description can contain "/*" or "*/" which will break
the generated block comment; before trimming/splitting the description in
formatComment, sanitize the description string (e.g., replace "/*" with "/ *"
and "*/" with "* /" or another safe replacement) so the block opener/closer are
not present in the content, then proceed to compute lines and render single-line
(LINE_COMMENT_PREFIX) or multi-line (BLOCK_COMMENT_START/BLOCK_COMMENT_END)
comments as before; update formatComment to perform this replacement on the
description variable prior to using it.
🧹 Nitpick comments (1)
protographic/src/sdl-to-mapping-visitor.ts (1)

135-140: Remove the stale TODO comment.
The required-fields mapping is now implemented, so this TODO is misleading.

🧹 Suggested cleanup
-            // todo: add required fields mapping

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
protographic/tests/sdl-to-proto/07-argument-ordering.test.ts (1)

375-378: ⚠️ Potential issue | 🟡 Minor

Potential bug: visitor3 uses modifiedSchema instead of modifiedSchema3.

Line 376 creates visitor3 with modifiedSchema (the second schema), but the test comment and modifiedSchema3 definition at lines 356-373 suggest it should use modifiedSchema3 (with re-added id field and new created field). This means the test isn't actually testing the third schema evolution scenario described in the comments.

🐛 Proposed fix
       // Create a third visitor using the same lock data
-      const visitor3 = new GraphQLToProtoTextVisitor(modifiedSchema, {
+      const visitor3 = new GraphQLToProtoTextVisitor(modifiedSchema3, {
         serviceName: 'UserService',
         lockData: lockData || undefined,
       });
🤖 Fix all issues with AI agents
In `@protographic/src/required-fields-visitor.ts`:
- Around line 133-138: The loop over this.keyDirectives is currently treating
every `@key` equally; update the iteration in required-fields-visitor (where
keyDirective is examined and getKeyFieldsString is called) to first inspect
keyDirective.arguments for an argument named "resolvable" and skip (continue) if
that argument exists and its value is the BooleanValue false, so non-resolvable
`@key` directives are not used to generate required-field RPCs; keep using
getKeyFieldsString, and throw the existing error only after confirming the
directive is resolvable.
🧹 Nitpick comments (8)
protographic/.eslintrc (2)

3-5: Normalize indentation in the plugins array.
Current spacing is inconsistent and hurts readability; align it with the rest of the JSON.


6-27: Avoid disabling no-undef and unused-vars globally.
These rules catch real runtime defects; consider scoping them to specific files via overrides instead of turning them off globally.

protographic/src/operations/message-builder.ts (2)

451-469: Inconsistent switch statement formatting.

This switch statement has different formatting than the one in collectFields (lines 128-170):

  • case statements appear to lack indentation relative to switch
  • Blank lines before break statements (lines 454, 460, 465)
  • break appears at reduced indentation

Compare with collectFields which has properly indented cases and no blank lines before break. Since the commit mentions ESLint compliance, consider making this consistent throughout the file.

🔧 Suggested formatting to match collectFields style
       switch (selection.kind) {
-      case 'Field': {
+        case 'Field': {
           processFieldSelection(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath);
-      
-      break;
-      }
-      case 'InlineFragment': {
+          break;
+        }
+        case 'InlineFragment': {
           // Nested inline fragment
           processInlineFragment(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath);
-      
-      break;
-      }
-      case 'FragmentSpread': {
+          break;
+        }
+        case 'FragmentSpread': {
           processFragmentSpread(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath);
-      
-      break;
-      }
-      // No default
+          break;
+        }
       }

518-537: Same formatting inconsistency as above.

This switch statement has the same formatting issues as processInlineFragment: inconsistent indentation and blank lines before break statements. Apply the same fix pattern for consistency.

protographic/src/proto-lock.ts (1)

106-136: Dead code: the reserved number reuse logic can never execute.

The code at lines 110-114 searches removedItems for a matching item name, but this condition can never be true:

  1. removedItems is populated at lines 67-71 with items that are not in availableItems
  2. The outer loop at line 106 iterates over availableItems
  3. Therefore, any item in availableItems cannot exist in removedItems

The condition removedItem === item will always be false, making the entire if (reservedNumber === undefined) branch at lines 116-124 always execute, and the else branch at lines 125-133 never execute.

This matches the test expectation that re-added fields get new numbers (not reusing reserved ones), so no bug in behavior. However, the code suggests an intention to reuse reserved numbers that doesn't work.

Consider either:

  1. Removing the dead code (lines 109-114 and 125-133) for clarity
  2. Fixing the logic if reserved number reuse is actually desired
♻️ Simplified version without dead code
     // Assign numbers to items that don't have one
     for (const item of availableItems) {
       if (newFieldsMap[item] === undefined) {
-        // Check if the item was previously removed (exists in our reservedNumbers)
-        let reservedNumber: number | undefined;
-        for (const [removedItem, number] of Object.entries(removedItems)) {
-          if (removedItem === item && reservedNumbers.includes(number)) {
-            reservedNumber = number;
-          }
-        }
-
-        if (reservedNumber === undefined) {
-          // Find next available number
-          let nextNumber = maxNumber + 1;
-          while (reservedNumbers.includes(nextNumber)) {
-            nextNumber++;
-          }
-
-          newFieldsMap[item] = nextNumber;
-          maxNumber = nextNumber;
-        } else {
-          // Reuse the reserved number for this item
-          newFieldsMap[item] = reservedNumber;
-
-          // Remove from reserved list
-          const index = reservedNumbers.indexOf(reservedNumber);
-          if (index !== -1) {
-            reservedNumbers.splice(index, 1);
-          }
-        }
+        // Find next available number
+        let nextNumber = maxNumber + 1;
+        while (reservedNumbers.includes(nextNumber)) {
+          nextNumber++;
+        }
+
+        newFieldsMap[item] = nextNumber;
+        maxNumber = nextNumber;
       }
     }
protographic/tests/sdl-to-proto/06-field-ordering.test.ts (1)

3-3: Unused import: isNull from lodash-es.

The isNull function is imported but never used in this file. This appears to be a leftover from refactoring.

🧹 Suggested fix
-import { isNull } from 'lodash-es';
protographic/src/sdl-to-mapping-visitor.ts (2)

2-2: Unused import: ArgumentNode.

The ArgumentNode type is imported but doesn't appear to be used in this file.

🧹 Suggested fix
 import {
-  ArgumentNode,
   DirectiveNode,
   GraphQLEnumType,

139-139: Stale TODO comment.

The TODO comment "todo: add required fields mapping" appears to be outdated since createRequiredFieldsMapping(type) is called immediately after the key directive loop at line 143.

🧹 Suggested fix
           if (key) {
             // Create entity mapping for each key combination
             this.createEntityMapping(typeName, key);
-            // todo: add required fields mapping
           }

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
protographic/src/operations/message-builder.ts (1)

129-169: ⚠️ Potential issue | 🟠 Major

Fail fast on proto-field name collisions to avoid silent drops.
graphqlFieldToProtoField can map distinct GraphQL fields to the same proto name (e.g., fooBar vs foo_bar). The current logic silently keeps the first and drops the rest, which can corrupt the generated schema. Consider detecting collisions and throwing with a clear error.

Suggested fix
           if (isObjectType(currentType) || isInterfaceType(currentType)) {
             const fieldName = selection.name.value;
             const protoFieldName = graphqlFieldToProtoField(fieldName);
-            if (!fieldNames.includes(protoFieldName)) {
+            const existing = fieldSelections.get(protoFieldName);
+            if (existing && existing.selection.name.value !== fieldName) {
+              throw new Error(
+                `Field name collision after proto mapping: "${existing.selection.name.value}" and ` +
+                  `"${fieldName}" both map to "${protoFieldName}".`,
+              );
+            }
+            if (!existing) {
               fieldNames.push(protoFieldName);
               fieldSelections.set(protoFieldName, { selection, type: currentType });
             }
           }
🤖 Fix all issues with AI agents
In `@protographic/src/sdl-to-proto-visitor.ts`:
- Around line 1555-1564: The field-filtering logic in the Sdl-to-Proto visitor
is incorrectly excluding fields annotated with REQUIRES_DIRECTIVE_NAME; update
the filter that builds validFields (which currently uses
allFields.filter(...).filter(...)) to only exclude fields with the
EXTERNAL_DIRECTIVE_NAME (remove REQUIRES_DIRECTIVE_NAME from the directive
check), so fields with `@requires` remain included in the object message
generation (refer to variables allFields and validFields and the constants
EXTERNAL_DIRECTIVE_NAME and REQUIRES_DIRECTIVE_NAME to locate the code).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@protographic/src/proto-utils.ts`:
- Around line 60-77: Inside buildProtoMessageWithIndent the composite-type scope
currently reuses the parent seenTypes causing leaks; create a per-message scope
Set (e.g., messageSeenTypes = new Set<string>()) and use that instead of the
parent seenTypes when building composite types and when recursing into nested
messages (pass messageSeenTypes to buildCompositeTypeMessage and to recursive
buildProtoMessageWithIndent calls) so each message has its own composite-type
scope and the parent set is not mutated or leaked across siblings.
🧹 Nitpick comments (1)
protographic/src/proto-utils.ts (1)

323-346: Wrapper example doesn’t match actual output.

buildWrapperMessage always emits a nested List message and List list = 1; even for level 1, but the example shows a direct repeated field. Update the docs (or adjust the implementation) so they align.

✏️ Doc alignment
- * For level 1: Creates simple wrapper like:
- *   message ListOfString {
- *     repeated string items = 1;
- *   }
+ * For level 1: Creates wrapper like:
+ *   message ListOfString {
+ *     message List {
+ *       repeated string items = 1;
+ *     }
+ *     List list = 1;
+ *   }

Comment on lines +60 to +77
// Build nested messages first - use a new Set for this level since
// protobuf allows the same message name at different nesting levels
if (message.nestedMessages && message.nestedMessages.length > 0) {
const nestedSeenTypes = new Set<string>();
for (const nestedMessage of message.nestedMessages) {
messageLines.push(...buildProtoMessageWithIndent(includeComments, nestedMessage, indent + 1, nestedSeenTypes));
}
}

if (message.reservedNumbers && message.reservedNumbers.length > 0) {
messageLines.push(indentContent(indent + 1, `reserved ${message.reservedNumbers};`));
}

const compositeTypeFields = message.fields.filter((f) => f.compositeType);
for (const compositeTypeField of compositeTypeFields) {
messageLines.push(
...buildCompositeTypeMessage(includeComments, compositeTypeField.compositeType!, indent + 1, seenTypes),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Composite type scope tracking leaks across nested messages.

buildProtoMessageWithIndent uses the parent seenTypes for composite types while nested messages use a fresh set. This lets composite type names bleed across sibling/nested scopes and can suppress required composite definitions (or miss collisions) within the same message. Use a per-message scope set for both nested messages and composite types.

💡 Suggested fix
-  // Build nested messages first - use a new Set for this level since
-  // protobuf allows the same message name at different nesting levels
-  if (message.nestedMessages && message.nestedMessages.length > 0) {
-    const nestedSeenTypes = new Set<string>();
-    for (const nestedMessage of message.nestedMessages) {
-      messageLines.push(...buildProtoMessageWithIndent(includeComments, nestedMessage, indent + 1, nestedSeenTypes));
-    }
-  }
+  // Build nested messages first - use a new Set for this level since
+  // protobuf allows the same message name at different nesting levels
+  const nestedSeenTypes = new Set<string>();
+  if (message.nestedMessages && message.nestedMessages.length > 0) {
+    for (const nestedMessage of message.nestedMessages) {
+      messageLines.push(...buildProtoMessageWithIndent(includeComments, nestedMessage, indent + 1, nestedSeenTypes));
+    }
+  }

   const compositeTypeFields = message.fields.filter((f) => f.compositeType);
   for (const compositeTypeField of compositeTypeFields) {
     messageLines.push(
-      ...buildCompositeTypeMessage(includeComments, compositeTypeField.compositeType!, indent + 1, seenTypes),
+      ...buildCompositeTypeMessage(includeComments, compositeTypeField.compositeType!, indent + 1, nestedSeenTypes),
     );
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Build nested messages first - use a new Set for this level since
// protobuf allows the same message name at different nesting levels
if (message.nestedMessages && message.nestedMessages.length > 0) {
const nestedSeenTypes = new Set<string>();
for (const nestedMessage of message.nestedMessages) {
messageLines.push(...buildProtoMessageWithIndent(includeComments, nestedMessage, indent + 1, nestedSeenTypes));
}
}
if (message.reservedNumbers && message.reservedNumbers.length > 0) {
messageLines.push(indentContent(indent + 1, `reserved ${message.reservedNumbers};`));
}
const compositeTypeFields = message.fields.filter((f) => f.compositeType);
for (const compositeTypeField of compositeTypeFields) {
messageLines.push(
...buildCompositeTypeMessage(includeComments, compositeTypeField.compositeType!, indent + 1, seenTypes),
);
// Build nested messages first - use a new Set for this level since
// protobuf allows the same message name at different nesting levels
const nestedSeenTypes = new Set<string>();
if (message.nestedMessages && message.nestedMessages.length > 0) {
for (const nestedMessage of message.nestedMessages) {
messageLines.push(...buildProtoMessageWithIndent(includeComments, nestedMessage, indent + 1, nestedSeenTypes));
}
}
if (message.reservedNumbers && message.reservedNumbers.length > 0) {
messageLines.push(indentContent(indent + 1, `reserved ${message.reservedNumbers};`));
}
const compositeTypeFields = message.fields.filter((f) => f.compositeType);
for (const compositeTypeField of compositeTypeFields) {
messageLines.push(
...buildCompositeTypeMessage(includeComments, compositeTypeField.compositeType!, indent + 1, nestedSeenTypes),
);
}
🤖 Prompt for AI Agents
In `@protographic/src/proto-utils.ts` around lines 60 - 77, Inside
buildProtoMessageWithIndent the composite-type scope currently reuses the parent
seenTypes causing leaks; create a per-message scope Set (e.g., messageSeenTypes
= new Set<string>()) and use that instead of the parent seenTypes when building
composite types and when recursing into nested messages (pass messageSeenTypes
to buildCompositeTypeMessage and to recursive buildProtoMessageWithIndent calls)
so each message has its own composite-type scope and the parent set is not
mutated or leaked across siblings.

@asoorm asoorm self-requested a review February 4, 2026 22:00
* These wrapper types allow distinguishing between unset fields and zero values
* in Protocol Buffers, which is important for GraphQL nullable semantics.
*/
export const SCALAR_WRAPPER_TYPE_MAP: Record<string, string> = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCALAR_TYPE_MAP and SCALAR_WRAPPER_TYPE_MAP already exist in operations/type-mapper.ts:20-37 - can we dedupe? Happy for this file to be the source of truth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants